execution/cache: prevent dead-fork StateCache fills across unwind - #23005

Review of the current head, limited to correctness bugs and memory issues.

1. branchCache fill is not covered by the staged-unwind guard

db/state/execctx/domain_shared.go:1508

The new guard was added to the StateCache fill (maxStep == kv.NoStepBound, line 1492), but the sibling branchCache.Put a few lines below is on the same read path and stays unguarded. branchCache is aggregator-scoped (line 299), so a bounded commitment read during a staged unwind inserts a staged-view value into a cache shared with the canonical generation.

SharedDomains.Unwind (line 849) only drops branch entries, which is safe in either direction. Inserting a value read under a step bound is not symmetric: the bound is step-granular, the unwind point is not, and nothing re-invalidates the entry if validation is rolled back — the durable rows were never unwound, but the cache now holds the bounded value with a txN below the floor, so it survives the epoch bump. That is the failure class the StateCache guard closes. Commit does not repeat a branch-cache invalidation at the durable boundary either; it only Puts/Invalidates the flushed commitment keys.

2. SuspendWarmup blocks uncancellably

execution/exec/blocks_read_ahead.go:146

SuspendWarmup is a bare warmupGate.Lock(). It replaced drainReadAhead, which called WaitForWarmup(ctx) and returned on ctx.Done(). If a warmBody worker is stuck on a slow MDBX read or a pinned snapshot, updateForkChoice (forkchoice.go:363), SetHead (set_head.go:61) and the validation path park forever while holding the exec semaphore, with no shutdown escape. The engine API stops answering instead of timing out.

3. A queued warmup can outlive shutdown's bounded wait

execution/exec/blocks_read_ahead.go:127

warmWg.Go registers the warmup, and only then does the goroutine park in withWarmupPermit on warmupGate.RLock(). At shutdown WaitForWarmup(warmCtx) (5s timeout) returns while that goroutine is still parked. When the suspension is released, warmBody runs db.BeginRo(ctx) after chainDB.Close() has begun — the waitTxsAllDoneOnClose hang WaitForWarmup exists to prevent. Before, the WaitGroup only covered warmups that were already running and would finish on their own.

4. publishing is cleared without a defer

execution/cache/state_cache.go:491 / :531

beginPublication sets publishing = true; only the final finishPublication resets it, on the normal path. If applyPrepared panics and the panic is recovered up the stack, publishing stays true for the rest of the process: eligibleFrontierLocked returns nil for every View, and fillIfFresh / fillCodeIfFresh / seedAddrCodeHash all short-circuit. The cache silently degrades to apply-only with nothing surfaced. applierMu is released by its own defer, so it is not even detectably wedged.

5. publish clones every value a second time, at the commit memory peak

execution/cache/state_cache.go:513

SharedDomains.Commit already deep-copies each flushed tuple into pendingState (append([]byte(nil), k...) / ...v...). publish then allocates a parallel make([]preparedStateUpdate, len(updates)) and prepareStateUpdate does bytes.Clone(update.Value) again. On a mainnet flush that holds two full copies of the flushed value bytes plus a second 40-byte-per-entry slice live at once, exactly at the commit peak. The old Applier.Apply path cloned once, inside putOrDelete.

The clone also runs before beginPublication, so a rejected publication pays for the whole copy. Moving the committedStateVersion <= sourceStateVersion check into beginPublication made this strictly worse: that early return used to precede the clone loop.

6. baseStateVersion is never refreshed, so Commit is silently single-use

db/state/execctx/domain_shared.go:1132

After a successful Commit the durable version is base+1 but sd.baseStateVersion is still base. A second Commit on the same SharedDomains fails stateVersionsForCommit with state version changed since SharedDomains was created, and ProjectedStateVersion() keeps returning base+1 — which dispatchNotificationsFromOverlay feeds to accumulator.SetStateID. Same after BlockOverlay().UpdateTxn(newerRoTx), which rebinds reads to a newer durable snapshot without touching baseStateVersion. Every current caller happens to rotate the SD, so the constraint is invisible until someone reuses one; the Commit docstring documents only the tx requirement.

7. Detached overlay reports every untouched sequence as 0

db/kv/membatchwithdb/memory_mutation.go:217

Dropping initSequences means the overlay no longer snapshots kv.Sequence. The readTx refactor fixed read views, but on a detached overlay readTx == nil and readSequenceLocked returns 0, nil. rawdb.WriteRawBodyIfNotExists would then take baseTxnID 0 from IncrementSequence(kv.EthTx, n) and ResetSequence on top of it; Flush copies kv.Sequence verbatim into the destination tx, rewinding the durable EthTx sequence. Latent — DetachDB has no production caller — but the previous code was safe by construction.

Smaller, same pass:

• execution/execmodule/forkchoice.go:363 and set_head.go:61 still take the exclusive gate for the whole call, so read-ahead warming is off during every FCU, not only the ones that unwind — SetHead holds it across BeginTemporalRw, i.e. while waiting for the MDBX writer lock. ValidateChain got the lazy ensureReadAheadSuspended treatment; these two did not.
• db/state/execctx/domain_shared.go:1660 — the hasUnwindBound branch calls getLatestMetered, whose first two steps are the sd.mem / sd.parent.mem lookups the lines just above already did and discarded, including both latestStateLock.RLocks. Passing the computed bound down would avoid two locked lookups per bounded account probe on the EVM hot path.
• db/state/execctx/domain_shared.go:193 — cacheFrontierFor boxes twice (sdFrontier into the embedded Frontier, then frontierWithStateVersion into the return), and when generationTx.ViewID() != sd.baseViewID it calls rawdb.GetStateVersion(generationTx), a cursor seek on kv.Sequence. Both per-miss rebind paths (lines 1500 and 1713) hit it, so every cache miss there pays a DB read the old code did not.

Two earlier findings are addressed at this head and need no action: the publication-rejection path now keeps the unwind authoritative via unwindLocked, and read views carry their own readTx.

Thanks for the detailed pass. Disposition on the current head (ba03dd895b):

• Point 1 is valid, but the bounded speculative-unwind fill is in the separate aggregator-scoped BranchCache and already exists on main. I moved it to execution/commitment: reject BranchCache fills from staged-unwind reads #23253, with the abandoned-validation and successful-unwind cases required as regression tests. This PR keeps its scope on StateCache fill readmission.
• Points 2 and 3 are fixed by eb41519663. One semaphore permit now represents either an active warmup or unwind suspension. Warmups use TryAcquire before starting a goroutine, so they skip instead of queueing. Suspension uses the caller's context, and FCU, SetHead, and ValidateChain abort before unwind if acquisition is cancelled. WaitForWarmup uses the same permit.
• Point 4 was considered and prototyped, then deliberately left out. A cache Put has no expected production panic path, and the StateStep recovery does not surround post-commit StateCache.Publish. Recovering only the cache would not make the broader post-commit operation recoverable. The proposed cleanup added a new exceptional state transition for a synthetic failure mode, so I do not think it belongs in this already large correctness PR.
• Point 5 is valid performance work and is tracked in execution/cache: avoid duplicate work when publishing StateCache batches #23226. It needs an explicit ownership contract and measurements rather than removing a defensive copy locally.
• Point 6 is now explicit in the SharedDomains.Commit contract: commit is terminal for that SharedDomains; callers continue with a new instance on a fresh transaction. Current production callers already rotate it this way.
• Point 7 is fixed by ba03dd895b. A bare detached overlay now returns an error when an untouched sequence needs the missing backing transaction, so IncrementSequence cannot silently start at zero. Explicit overlay sequence writes still work, and NewReadView(tx) remains the supported way to resolve untouched sequences. The regression test was red before the change and also verifies that a failed increment does not create a zero-based value.

For the smaller items:

• Narrowing FCU read-ahead exclusion to actual unwinds remains the performance-only follow-up in execution/execmodule: drain read-ahead only before an actual unwind #23003.
• The repeated mem lookup occurs only after a staged-unwind bound is observed. I left it alone because plumbing the bound through another read path adds complexity to a rare transition path, not the normal EVM hot path.
• 7386067f4f distinguishes retryable bindings from stale rejected views, so stale RPC transactions no longer repeat the state-version read on every miss. The SD's own transaction also uses its memoized version. Remaining direct-read/view-reuse work is tracked in execution/cache: avoid rebuilding cache views on direct reads #23145.

The two earlier items you noted remain fixed: rejected PublishUnwind calls still perform the durable invalidation (4c3a6b31c8), and overlay read views carry their own plain read transaction (452aab4700).

Verification for the latest changes: full db/kv/membatchwithdb tests under -race, repeated make lint, and make erigon integration.